Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: added support for daily and weekly active user aggregations #18101

Merged
merged 7 commits into from
Oct 20, 2023

Conversation

Gilbert09
Copy link
Member

Problem

Changes

  • Builds on to QueryBuilder, making it the orchestrator of putting the query together, with other classes returning AST nodes
  • Brings together breakdowns and aggregations
  • Adds a AggregationOperations class to organize the extra joins needed when doing the likes of weekly active users
    • New logic will likely be added to this next when working on monthly active users, along with tests to ensure the logic splits are sound
  • Added some pytests for the QueryModifier class. More are to come for the aggregation stuff soon

image

image

How did you test this code?

@Gilbert09 Gilbert09 requested a review from mariusandra October 19, 2023 15:31
@Gilbert09 Gilbert09 changed the title Added support for daily and weekly active user aggregations feat: added support for daily and weekly active user aggregations Oct 19, 2023
from posthog.schema import ActionsNode, EventsNode


class QueryModifier:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we can rename this to something else? We already have the system of "hogql query modifiers" inside modifier.py, which sets things like PoE on/off, cohorts via join/subquery, etc. To avoid confusing people, can we find something else for this? QueryAlternator 🙈?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, yep! I can change this - shame you can't scope classes to just the file they're defined in

Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more comments, but overall I think this works. Happy to merge and go on.

Comment on lines 35 to 43
def appendSelect(self, expr: ast.Expr) -> None:
self._selects.append(expr)

def appendGroupBy(self, expr: ast.Expr) -> None:
self._group_bys.append(expr)

def replaceSelectFrom(self, join_expr: ast.JoinExpr) -> None:
self._select_from = join_expr

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our adherence to this is a mixed bag... but normally python prefers snake_case method names

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, that's me falling into JS habits 🤦 Not intentional - will change

from posthog.schema import ActionsNode, EventsNode


class QueryAlternator:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to leave a line of prose for the future traveller, as to what this class does, without having to go through all the code.

@Gilbert09 Gilbert09 merged commit af0c46a into master Oct 20, 2023
67 checks passed
@Gilbert09 Gilbert09 deleted the feat/trends-weekly-active branch October 20, 2023 13:39
daibhin pushed a commit that referenced this pull request Oct 23, 2023
…8101)

* Added support for daily and weekly active user aggregations

* Removed unused join

* Fixed tests and applied Sample placeholders

* Fixed formula running on an empty string

* Updated the name of QueryModifier

* Comments and fixed name casing
Justicea83 pushed a commit to Justicea83/posthog that referenced this pull request Oct 24, 2023
…stHog#18101)

* Added support for daily and weekly active user aggregations

* Removed unused join

* Fixed tests and applied Sample placeholders

* Fixed formula running on an empty string

* Updated the name of QueryModifier

* Comments and fixed name casing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants